refactor(security): improve SecureRandom implementation, tests, and docs#56224
Open
joshtrichards wants to merge 7 commits into
Open
refactor(security): improve SecureRandom implementation, tests, and docs#56224joshtrichards wants to merge 7 commits into
joshtrichards wants to merge 7 commits into
Conversation
joshtrichards
added
enhancement
tests
joshtrichards
requested review from
ArtificialOwl,
CarlSchwan,
icewind1991 and
leftybournes
and removed request for
a team
Comment on lines
+93
to
+95
| /** | ||
| * @dataProvider invalidCharProviders | ||
| */ |
Member
There was a problem hiding this comment.
Suggested change
| /** | |
| * @dataProvider invalidCharProviders | |
| */ | |
| #[DataProvider('invalidCharProviders') |
(and add the class import)
|
|
||
| public function testDefaultCharsetBase64Characters(): void { | ||
| $randomString = $this->rng->generate(100); | ||
| $this->assertMatchesRegularExpression('/^[A-Za-z0-9\+\/]+$/', $randomString); |
Member
There was a problem hiding this comment.
Suggested change
| $this->assertMatchesRegularExpression('/^[A-Za-z0-9\+\/]+$/', $randomString); | |
| $this->assertMatchesRegularExpression('/^[A-Za-z0-9\+\/]{100}$/', $randomString); |
Member
Author
There was a problem hiding this comment.
Done - apologies for the delay!
Comment on lines
+116
to
+117
| $this->assertMatchesRegularExpression('/^[abcd]+$/', $randomString); | ||
| $this->assertEquals(500, strlen($randomString)); |
Member
There was a problem hiding this comment.
Suggested change
| $this->assertMatchesRegularExpression('/^[abcd]+$/', $randomString); | |
| $this->assertEquals(500, strlen($randomString)); | |
| $this->assertMatchesRegularExpression('/^[abcd]{500}$/', $randomString); |
Or keep the length check separate but then you can also do that upstairs.
| public function testUserProvidedValidCharset(): void { | ||
| $charset = '@#$!'; | ||
| $randomString = $this->rng->generate(64, $charset); | ||
| $this->assertMatchesRegularExpression('/^[@#$!]+$/', $randomString); |
Member
There was a problem hiding this comment.
Suggested change
| $this->assertMatchesRegularExpression('/^[@#$!]+$/', $randomString); | |
| $this->assertMatchesRegularExpression('/^[@#$!]{64}$/', $randomString); |
| * @throws \LengthException If $length <= 0. | ||
| * @throws \InvalidArgumentException if $characters contains non-ASCII characters, duplicates, | ||
| * or fewer than 4 unique characters. | ||
| * @since 8.0.0 |
Member
There was a problem hiding this comment.
Suggested change
| * @since 8.0.0 | |
| * @since 35.0.0 $characters has to be at least 4 chars long, non-AS… etc | |
| * @since 8.0.0 |
Add a @since for your change to the rules of the method
- Add constant for default $characters set - Update docs for accuracy and clarity Signed-off-by: Josh <josh.t.richards@gmail.com>
- Use a constant for the default $characters set (still same chars) - Add more robust reasonableness checks for $characters set - Switch docblocks to point at interface docs to avoid duplication Signed-off-by: Josh <josh.t.richards@gmail.com>
New test coverage for: - charset validation failures (duplicates, too short, non-ASCII). - default charset (CHAR_BASE64_RFC4648). - randomness/unique output. - minimum-sized valid charsets and large printable ASCII charset. - a caller-provided, valid (non-predefined) custom charset. And adjusts the length ranges tests: - Added some smaller / more frequently used ranges. - Added an "oddball" range to possibly catch weird stuff. - Dropped excessive 64K test which does not need to run within our CI during standard PR runs (if deemed desirable maybe add it to an occasional stress tester, but I don't think it's necessary for now) Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
Signed-off-by: Josh <josh.t.richards@gmail.com>
joshtrichards
force-pushed
the
jtr/refactor-SecureRandom
branch
from
f2cdc9b to
971a1fe
Compare
Signed-off-by: Josh <josh.t.richards@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves the SecureRandom implementation along with its related tests and documentation.
Key changes
Refactored implementation:
$charactersset passed togenerate().ISecureRandomandSecureRandomto better describe behavior, parameters, and usage.Expanded and updated tests:
TODO
Checklist
3. to review, feature component)stable32)